Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: wsts-related cleanup #1287

Merged
merged 16 commits into from
Feb 5, 2025
Merged

refactor: wsts-related cleanup #1287

merged 16 commits into from
Feb 5, 2025
+657 −306

Conversation

cylewitruk
Copy link
Member

@cylewitruk cylewitruk commented Jan 30, 2025 •
edited
Loading

Description

A breakout PR from #1285 to move some of the more refactor-like changes related to WSTS.

Part of: #1300

⚠️ This PR's branch is targeted by #1285 which needs to merge into this branch before merging this PR into main.

Changes

  • Rename WstsMessage.txid to id and use a WstsMessageId enum instead of a bitcoin::Txid.
    • Protobufs updated in a backwards-compatible way, deprecating the txid field (flagging this breaking-protocol anyway, just in-case).
    • Generated proto code updated.
  • StateMachineId is now an enum instead of a newtype over [u8; 32].
  • A fair bit of restructuring/cleanup in wsts_state_machine.rs. This is the bulk of the changes in this PR
  • Some logging improvements.
  • Updates wsts to this commit (apply-fix + this).
    • ... and brings FROST back to life 🥳

Testing Information

All existing tests pass and devenv works like a charm.

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@cylewitruk cylewitruk added the breaking-protocol Breaking protocol changes label Jan 30, 2025
@cylewitruk cylewitruk added this to the sBTC: Deposits milestone Jan 30, 2025
@cylewitruk cylewitruk self-assigned this Jan 30, 2025
@cylewitruk cylewitruk mentioned this pull request Jan 30, 2025
Open
4 tasks
@cylewitruk cylewitruk requested a review from xoloki February 3, 2025 16:31
xoloki
xoloki approved these changes Feb 3, 2025
View reviewed changes
Copy link
Collaborator

@xoloki xoloki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of quick questions, but LGTM

signer/src/transaction_signer.rs Outdated Show resolved Hide resolved
signer/src/transaction_signer.rs Show resolved Hide resolved
@cylewitruk
Copy link
Member Author

@xoloki is the wsts version/rev correct?

@cylewitruk cylewitruk mentioned this pull request Feb 4, 2025
Open
1 task
cylewitruk
cylewitruk commented Feb 4, 2025
View reviewed changes
signer/src/message.rs Outdated Show resolved Hide resolved
@cylewitruk cylewitruk mentioned this pull request Feb 4, 2025
Draft
4 tasks
matteojug
matteojug reviewed Feb 4, 2025
View reviewed changes
signer/src/proto/convert.rs Outdated Show resolved Hide resolved
signer/src/proto/convert.rs Outdated Show resolved Hide resolved
signer/src/proto/convert.rs Show resolved Hide resolved
signer/src/testing/wsts.rs Outdated Show resolved Hide resolved
signer/src/transaction_coordinator.rs Outdated Show resolved Hide resolved
signer/src/transaction_signer.rs Show resolved Hide resolved
signer/src/transaction_signer.rs Outdated Show resolved Hide resolved
signer/src/transaction_signer.rs Show resolved Hide resolved
signer/src/message.rs Show resolved Hide resolved
matteojug
matteojug reviewed Feb 4, 2025
View reviewed changes
signer/src/wsts_state_machine.rs Outdated Show resolved Hide resolved
signer/src/wsts_state_machine.rs Show resolved Hide resolved
signer/src/wsts_state_machine.rs Outdated Show resolved Hide resolved
signer/src/wsts_state_machine.rs Outdated Show resolved Hide resolved
signer/src/wsts_state_machine.rs Outdated Show resolved Hide resolved
@xoloki
Copy link
Collaborator

xoloki commented Feb 4, 2025 •
edited
Loading

@xoloki is the wsts version/rev correct?

This is the rev for the head of the apply-fix branch, which has all the hotfixes plus the framework necessary for this PR.

I'm really not sure what version this should be. That branch is still version 10.0.0, but we changed the Coordinator trait in the last two changes, so maybe it needs to be 10.1.0 ?

@stacks-network stacks-network deleted a comment from matteojug Feb 4, 2025
@cylewitruk
Copy link
Member Author

@xoloki is the wsts version/rev correct?

This is the rev for the head of the apply-fix branch, which has all the hotfixes plus the framework necessary for this PR.

I'm really not sure what version this should be. That branch is still version 10.0.0, but we changed the Coordinator trait in the last two changes, so maybe it needs to be 10.1.0 ?

Yeah we might need a minor release to include that, if we don't want to use the commit hash. I've reviewed the changes so I'm fine with using the commit hash until it's ready, but yeah it would be nice to have a 10.1.0.

matteojug
matteojug approved these changes Feb 5, 2025
View reviewed changes
signer/src/proto/convert.rs Outdated Show resolved Hide resolved
@xoloki
Copy link
Collaborator

xoloki commented Feb 5, 2025

Yeah we might need a minor release to include that, if we don't want to use the commit hash. I've reviewed the changes so I'm fine with using the commit hash until it's ready, but yeah it would be nice to have a 10.1.0.

Ok, I'll publish 10.1.0 just before you merge this set of PRs into main, to avoid any churn. Or I could just do it now and risk needing to also publish 10.2.0.

@cylewitruk cylewitruk changed the title refactor: breakout from #1285 - wsts-related cleanup refactor: wsts-related cleanup Feb 5, 2025
@cylewitruk cylewitruk merged commit dc219d7 into main Feb 5, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xoloki xoloki xoloki approved these changes

@djordon djordon djordon approved these changes

@matteojug matteojug matteojug approved these changes

Assignees

@cylewitruk cylewitruk

Labels
breaking-protocol Breaking protocol changes
Projects
sBTC
Status: Done
Milestone
sBTC: Deposits
Development

Successfully merging this pull request may close these issues.
4 participants
@cylewitruk @xoloki @matteojug @djordon